Conversation
WyriHaximus
force-pushed
the
close-inactive-connections
branch
4 times, most recently
from
1592cb1 to
0647b58
Compare
| if ($data !== '') { | ||
| $loop->cancelTimer($timer); | ||
| } | ||
| }); |
There was a problem hiding this comment.
@WyriHaximus This looks like a good starting point 👍
However, the way it's currently designed, this will only wait for some data and then wait infinitely for a complete HTTP request header. I would suggest moving this logic to the RequestHeaderParser and make the timeout apply to the entire HTTP request header. For instance, if I send GET / HTTP/1.1\r\n but nothing else, this should still run into a timeout.
There was a problem hiding this comment.
@clue Correct, that is intentional. My initial intent was to first create this PR and do it on a connection level, and then do a follow up to do it on both the connection and request level. But if you think it would be better to do both I can fold that into this PR.
There was a problem hiding this comment.
@WyriHaximus Without knowing the complete scope of this feature I would lean towards including the timeout on the request level, but I'll leave this up to you to decide 👍
Either way I think the above snippet should be updated without the if statement as the underlying socket and stream components should never emit an empty data event.
There was a problem hiding this comment.
@clue updated the PR with that change as per your suggestion. As discussed this morning I'll also file the follow-up PR taking care of the idle request closing.
WyriHaximus
force-pushed
the
close-inactive-connections
branch
from
0647b58 to
f33f3ac
Compare
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
| }); | ||
| $loop = $this->loop; | ||
| $conn->once('data', function () use ($loop, $timer) { | ||
| $loop->cancelTimer($timer); |
There was a problem hiding this comment.
Consider a 60 second timeout for all headers instead
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@WyriHaximus Thanks for the update!
Can you provide a quick update here, what's your plan for this feature with regards to #425? We've briefly discussed (off channel) that this should eventually send a 408 Request Timeout response for the initial connection but not for keep-alive connections that send no following request. I agree breaking this up into smaller PRs can be useful, but it looks like this might also work in a single PR, so I'll leave this up to you. Let me know if there's anything I can help with and/or if you want me to take over from here 👍
| $conn->on('end', function () use ($loop, $timer) { | ||
| $loop->cancelTimer($timer); | ||
| }); |
There was a problem hiding this comment.
This should be unneeded given the end event will always be followed by the close event which is handled the same way.
There was a problem hiding this comment.
Good point, will drop this one 👍
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This new middleware introduces a timeout of closing inactive connections between connections after a configured amount of seconds. This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus
force-pushed
the
close-inactive-connections
branch
from
f33f3ac to
5428717
Compare
@clue Will fold it into a single PR, and will be adding the 408 next 👍 |
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
This builds on top of reactphp#405 and further builds out reactphp#423 by also close connections with inactive requests.
2 participants
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.
This builds on top of #405 and partially on #422
This PR very specifically doesn't deal with inactivity while handling requests, which will be proposed in a different PR.